DEVPROD-10103 Account for cross-build dependents in NumDependents #8560
Conversation
model/lifecycle.go
Outdated
| if err = allTasks.InsertUnordered(ctx); err != nil { | ||
| return nil, errors.Wrap(err, "inserting tasks") | ||
| } | ||
| SetNumDependents(allTasks) |
There was a problem hiding this comment.
Should this be above the insert like here?
| @@ -738,6 +738,7 @@ func FinalizePatch(ctx context.Context, p *patch.Patch, requester string) (*Vers | |||
| }, | |||
| ) | |||
| } | |||
| SetNumDependents(tasksToInsert) | |||
There was a problem hiding this comment.
Why do we call this here now too?
There was a problem hiding this comment.
Because the CreateBuildFromVersionNoInsert calls that occur inside the loop no longer set the tasks num dependents field, we now need to do it right before we insert all tasks for the version
| @@ -980,6 +980,8 @@ func createVersionItems(ctx context.Context, v *model.Version, metadata model.Ve | |||
| }) | |||
| } | |||
|
|
|||
| model.SetNumDependents(tasksToCreate) | |||
There was a problem hiding this comment.
And here? This is just for my curiosity but if the reason is involved enough, it might be worth adding a comment since it doesn't seem to affect the adjacent logic
There was a problem hiding this comment.
Same rationale as the other comment
repotracker/repotracker_test.go
Outdated
| } | ||
| assert.NoError(t, previouslyActivatedVersion.Insert()) | ||
|
|
||
| // insert distros used in testing. |
There was a problem hiding this comment.
I don't think this needs a comment.
repotracker/repotracker_test.go
Outdated
| pp, err := model.LoadProjectInto(ctx, []byte(simpleYml), nil, "testproject", p) | ||
| assert.NoError(t, err) | ||
|
|
||
| // create new version to use for activating |
There was a problem hiding this comment.
| // create new version to use for activating | |
| // Create new version to use for activating. |
repotracker/repotracker_test.go
Outdated
| }, | ||
| }, | ||
| } | ||
| assert.NoError(t, previouslyActivatedVersion.Insert()) |
There was a problem hiding this comment.
Could you convert the ones checking for errors to requires for this test function? I think the only one that shouldn't be for sure is assert.NoError(t, repoTracker.StoreRevisions(ctx, revisions)) L376
| p := &model.Project{} | ||
| pp, err := model.LoadProjectInto(ctx, []byte(simpleYml), nil, "testproject", p) | ||
| assert.NoError(t, err) | ||
|
|
There was a problem hiding this comment.
| require.NotNil(t, pp) | |
Looking at LoadProjectInto, I don't think it can ever return a nil ParserProject when the error is not nil. I'm not sure if we should double check that in the test in case the function changes or it will be consistent. Feel free to decline this suggestion if you think so!
DEVPROD-10103
Description
The
NumDependentsfield for tasks can be quite undercounted because it does not currently consider cross-variant dependencies.NumDependentsfor tasks until all tasks for the version have been createdTesting
Updated unit tests to confirm cross-variant dependencies are part of the final calculation.